Skip to content

Conversation

@zachahn
Copy link

@zachahn zachahn commented Jan 6, 2024

Hello!

I wrote a tiny Ruby library that wraps this crate. I really like how fully featured your crate is!

This PR is very much WIP, but I wanted to hear your thoughts before I got any further. As a part of working on this PR, I had to compare this repo with the canonical PHP repo, and I did find the similarities quite helpful 😁

I pulled the most recent fixtures from php-textile into this library. The canonical PHP repo seemed to have just a few changes since 2022, so I didn't think there would be too many changes to bring it all up to date. But this import ended up breaking a lot of tests!

Here's a quick summary of the various problems I've been running into:

  1. The setup key is not a hash anymore, it is an array of hashes. I started to try to fix this, but I paused after hitting some ownership issues. Edit: I have an initial fix for this!
  2. The PHP repository strips trailing slashes / from links. I fixed at least one test here that had this issue.
  3. It seems like the PHP repo has a way of having a blank "UID". This repo seems to default the UID to xyz in tests. I didn't really look into this, I stopped once I got this far. This affects tests with footnotes.

What are your thoughts on this? Are you still maintaining this crate? Would you say you want these changes? And if so, do you have any thoughts on how we can make this library compatible with the PHP repo? Would you say that this is a goal of yours?

To be honest, I can't commit to getting this PR across the finish line. I don't expect you to either! Please do feel free to close this PR. But I am interested in some of these problems, so it's very possible I will make some progress on this here and there.

The way that this is implemented seems to be slightly different than the
PHP library handles it, so I'm not completely sure if I've edited all
the parts that need to be edited.
@kpot
Copy link
Owner

kpot commented Jan 7, 2024

@zachahn Hi!

I'm glad this project is useful and I'll be happy to merge any improvements or fixes you do!

Except I don't feel that distorting URL's is a good idea. If this is what PHP-Textile does, well, I strongly disagree with their approach. For instance, https://example.com/book and https://example.com/book/ are totally different URL, exactly because of the trailing slash /. The first one describes a book. The second one describe all the books available. This is a critical distinction in REST and many publication systems. So all URLs must be preserved in their original form, as much, as possible. With no misguided "improvements" that change their destination.

Other than that, if the general compatibility is preserved and the tests are green, feel free to improve anything. I'm ready to merge it.

@zachahn
Copy link
Author

zachahn commented Feb 29, 2024

After some thought, I realized that this PR would end up too large, and that I didn't see an easy way to break it into multiple changes.

Closing this one in favor of #2, which imports just the single change I need to unblock my work.

@zachahn zachahn closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants